feat: add a knob for reset stack by Duslia · Pull Request #4813 · bytecodealliance/wasmtime

Duslia · 2022-08-30T03:28:00Z

This PR has been discussed in #4637. I add a knob for reset stack.

github-actions · 2022-08-30T03:42:33Z

Subscribe to Label Action

cc @peterhuene

Details

This issue or pull request has been labeled: "wasmtime:api", "wasmtime:config"

Thus the following users have been cc'd because of the following labels:

peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

github-actions · 2022-08-30T03:42:48Z

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

If you added a new Config method, you wrote extensive documentation for
it.

Details

Our documentation should be of the following form:

Short, simple summary sentence.

More details. These details can be multiple paragraphs. There should be
information about not just the method, but its parameters and results as
well.

Is this method fallible? If so, when can it return an error?

Can this method panic? If so, when does it panic?

# Example

Optional example here.

If you added a new Config method, or modified an existing one, you
ensured that this configuration is exercised by the fuzz targets.

Details

For example, if you expose a new strategy for allocating the next instance
slot inside the pooling allocator, you should ensure that at least one of our
fuzz targets exercises that new strategy.

Often, all that is required of you is to ensure that there is a knob for this
configuration option in wasmtime_fuzzing::Config (or one
of its nested structs).

Rarely, this may require authoring a new fuzz target to specifically test this
configuration. See our docs on fuzzing for more details.
If you are enabling a configuration option by default, make sure that it
has been fuzzed for at least two weeks before turning it on by default.

Details

To modify this label's message, edit the .github/label-messager/wasmtime-config.md file.

To add new label messages or remove existing label messages, edit the
.github/label-messager.json configuration file.

Learn more.

alexcrichton

Thanks for this! Can you be sure to add some tests exercising this? Additionally can you update the fuzz configuration in crates/fuzzing/src/generators/config.rs to have a boolean to configure this flag?

While you're here, would you also be updating for perhaps updating the method names here? I originally thought that this wouldn't work on other platforms since it means that stacks are repeatedly called with commit_stack_pages which seems like it should error. In reality though Windows doesn't use this code path and additionally the Unix implementation of commit_stack_pages is a noop. In that case I think it's safe to remove commit_stack_pages entirely and otherwise rename decommit_stack_pages to something like reset_stack_pages_to_zero.

Actually though as I think more about this, I feel that there's a case to be made for removing this option entirely and simply not resetting stack pages back to zero. I don't think that the defense-in-depth argument holds a ton of water here because we make no attempt to reset the stack for non-async calls. If an alternate stack was always used then I think there's a case to be made for having this as an option since it would be turning off a uniformly off-by-default behavior, but otherwise as-is this option is only applicable with async support and the pooling instance allocator, both of which are already niche.

@cfallin I know in the past you've argued that this behavior should remain, so I'm curious if you still feel that way or if I could perhaps convince you of otherwise.

cfallin · 2022-08-30T15:45:16Z

Actually though as I think more about this, I feel that there's a case to be made for removing this option entirely and simply not resetting stack pages back to zero. I don't think that the defense-in-depth argument holds a ton of water here because we make no attempt to reset the stack for non-async calls. If an alternate stack was always used then I think there's a case to be made for having this as an option since it would be turning off a uniformly off-by-default behavior, but otherwise as-is this option is only applicable with async support and the pooling instance allocator, both of which are already niche.

@cfallin I know in the past you've argued that this behavior should remain, so I'm curious if you still feel that way or if I could perhaps convince you of otherwise.

I think it's pretty important to keep this behavior, for a few reasons:

Depending on regalloc and stack frame management code to be correct in order to not leak stale data from other instances feels way too risky to me. Yes, any regalloc bug could create a read gadget that reads anything in the address space... but having sensitive data from another instance immediately on our stack, in stack slots that haven't yet been overwritten, increases risk by a very significant amount. It just seems like too much to me to remove that layer of safety (zero out the sensitive data under the nose of the new instance) in cases where we are otherwise very careful about isolation.
Because of that, I want the option to run with zeroed stacks for new instances. However, I don't care too much if it's the default or not; there are a number of config options that folks running wasmtime in high-performance, high-security settings will want to consider anyway, and this is just one more such setting. But the option shouldn't go away, IMHO.
And actually I agree that the inconsistency between async and sync cases should be addressed, but in the opposite direction: it would be great if someday we could have the option to execute sync Wasm calls on a fresh zeroed stack as well! But almost certainly not by default because that's more of a significant penalty (async needs some separate stack whereas sync can run on the host stack otherwise).

alexcrichton · 2022-08-30T16:56:43Z

That sounds reasonable to me. @Duslia would you be ok implementing this new option, but flipping the defaults? Instead the option would enable the zeroing behavior that is currently the default today, and by default Wasmtime wouldn't zero stacks async stacks.

Duslia · 2022-08-31T13:06:53Z

I will add some tests tomorrow.

alexcrichton · 2022-08-31T15:04:29Z

        #[cfg(not(feature = "async"))]
        let stack_size = 0;

+        let _ = self.async_stack_zeroing;


I think it's ok to drop this as it's always used by the snippet below

alexcrichton · 2022-08-31T15:05:03Z

+    /// deallocation will simply release the stack back to the pool. During the deallocation
+    /// process Wasmtime will by default reset the contents of the stack back to zero.


This last sentence will need an update since this PR is also changing the defaults.

github-actions · 2022-09-01T08:38:44Z

Subscribe to Label Action

cc @fitzgen

Details

This issue or pull request has been labeled: "fuzzing"

Thus the following users have been cc'd because of the following labels:

fitzgen: fuzzing

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

alexcrichton · 2022-09-01T15:27:20Z

I pushed up some tweaks to the wording here but otherwise this looks good to me, thanks!

github-actions Bot added wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:config Issues related to the configuration of Wasmtime labels Aug 30, 2022

Duslia force-pushed the feat/knob_for_reset_stack branch from d9f4f57 to 0020cac Compare August 30, 2022 03:38

Duslia force-pushed the feat/knob_for_reset_stack branch from 0020cac to b736500 Compare August 30, 2022 07:34

bjorn3 reviewed Aug 30, 2022

View reviewed changes

Comment thread crates/wasmtime/src/config.rs Outdated

bjorn3 reviewed Aug 30, 2022

View reviewed changes

Comment thread crates/runtime/src/instance/allocator/pooling.rs

Duslia force-pushed the feat/knob_for_reset_stack branch 2 times, most recently from 0173e8d to d1fb240 Compare August 30, 2022 07:45

bjorn3 reviewed Aug 30, 2022

View reviewed changes

Comment thread .idea/workspace.xml Outdated

Duslia force-pushed the feat/knob_for_reset_stack branch 2 times, most recently from 03ea2cf to 391c309 Compare August 30, 2022 09:18

bjorn3 reviewed Aug 30, 2022

View reviewed changes

Comment thread crates/wasmtime/src/config.rs Outdated

Duslia force-pushed the feat/knob_for_reset_stack branch 4 times, most recently from 729fb4d to 38979f8 Compare August 30, 2022 10:27

alexcrichton reviewed Aug 30, 2022

View reviewed changes

Comment thread crates/wasmtime/src/config.rs Outdated

Comment thread crates/wasmtime/src/config.rs Outdated

Comment thread crates/wasmtime/src/config.rs Outdated

Duslia force-pushed the feat/knob_for_reset_stack branch 5 times, most recently from c8e507a to 761abd5 Compare August 31, 2022 10:36

Duslia changed the title ~~feat: add a knob for reset stack~~ [WIP]feat: add a knob for reset stack Aug 31, 2022

Duslia force-pushed the feat/knob_for_reset_stack branch 3 times, most recently from cd101cc to f956e73 Compare August 31, 2022 13:02

alexcrichton reviewed Aug 31, 2022

View reviewed changes

Duslia force-pushed the feat/knob_for_reset_stack branch 4 times, most recently from 4748a11 to 71fa050 Compare September 1, 2022 08:17

github-actions Bot added the fuzzing Issues related to our fuzzing infrastructure label Sep 1, 2022

Duslia force-pushed the feat/knob_for_reset_stack branch 2 times, most recently from fdb39fe to 78a1981 Compare September 1, 2022 09:29

Duslia changed the title ~~[WIP]feat: add a knob for reset stack~~ feat: add a knob for reset stack Sep 1, 2022

feat: add a knob for reset stack

1260c76

Duslia force-pushed the feat/knob_for_reset_stack branch from 78a1981 to 1260c76 Compare September 1, 2022 09:45

Touch up documentation of async_stack_zeroing

6f93f22

alexcrichton enabled auto-merge (squash) September 1, 2022 15:26

alexcrichton approved these changes Sep 1, 2022

View reviewed changes

alexcrichton merged commit bca4dae into bytecodealliance:main Sep 1, 2022

		/// deallocation will simply release the stack back to the pool. During the deallocation
		/// process Wasmtime will by default reset the contents of the stack back to zero.

Conversation

Duslia commented Aug 30, 2022

Uh oh!

github-actions Bot commented Aug 30, 2022

Subscribe to Label Action

Uh oh!

github-actions Bot commented Aug 30, 2022

Label Messager: wasmtime:config

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cfallin commented Aug 30, 2022

Uh oh!

alexcrichton commented Aug 30, 2022

Uh oh!

Duslia commented Aug 31, 2022

Uh oh!

alexcrichton Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

alexcrichton Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 1, 2022

Subscribe to Label Action

Uh oh!

alexcrichton commented Sep 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants